Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

fix(index): correct loader sourcemaps usage #67

Merged
merged 3 commits into from
Jul 12, 2017

Conversation

mattlewis92
Copy link
Member

I was wrong about the sourcemap behaviour, turns out webpack does actually pass the sourcemap as the second arg to loaders, this PR restores the previous behaviour (with a perf boost by not trying to parse for an inline sourcemap if there is already a source map available)

Depends on #66

@codecov
Copy link

codecov bot commented Jul 10, 2017

Codecov Report

Merging #67 into master will decrease coverage by 9.09%.
The diff coverage is 83.33%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #67     +/-   ##
========================================
- Coverage     100%   90.9%   -9.1%     
========================================
  Files           2       2             
  Lines           6      11      +5     
  Branches        0       2      +2     
========================================
+ Hits            6      10      +4     
- Misses          0       1      +1
Impacted Files Coverage Δ
src/index.js 90% <83.33%> (-10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0cf823...d8c2740. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title Fix sourcemaps fix(index): correct loader sourcemaps usage Jul 10, 2017
appveyor.yml Outdated
@@ -19,6 +19,7 @@ matrix:
fast_finish: true
install:
- ps: Install-Product node $env:nodejs_version x64
- cmd: npm i -g npm@latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixing the current falky builds for branches ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to fix appveyor on node 4, which uses npm 2.x which apparently isn't compatible with babel

import path from 'path';
import loader from '../../src/cjs';

const windowsToUnixPathSeparators = str => str.replace(/\\/g, '/');
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const normalize = str => str.split(path.sep).join('/')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@joshwiens
Copy link
Member

@mattlewis92 - Just landed #66

I'll take a look at appveyor and figure out why it's not reporting.

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appveyor should be sorted, needs a quick rebase & g2g

@mattlewis92 mattlewis92 reopened this Jul 12, 2017
@mattlewis92
Copy link
Member Author

@d3viant0ne done!

@joshwiens joshwiens merged commit 691b565 into webpack-contrib:master Jul 12, 2017
@mattlewis92 mattlewis92 deleted the fix-sourcemaps branch July 12, 2017 21:46
@mattlewis92
Copy link
Member Author

Thanks @d3viant0ne 😄 Gave the RC a spin in our app and its all working smoothly now. We probably want to land #65 before cutting 3.0.0 final, as that PR is breaking.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants